feat(plugin): support command's flags #3060

tbruyelle · 2022-11-04T14:22:39Z

This change brings support to command's flags by :

Adding a Flags fields to plugin.Command struct.
This allows the user to add flags to his plugin's command.
Expose a Flags() *pflag.Flagset function inside Execute* plugin method argument.
So the user can retrieve flags values like he used to do with cobra.
Make pflag.Flag serializable.
This is required because the plugin and the ignite binary communicate via the network. Since pflag.Flag has a lot of un-exported fields, serialization doesn't work out the box for this type. I had a create a subsidiary type for that.

Other changes:

Merge Commands() and Hooks() methods into a single Manifest method.
This simplifiy the declaration of what the plugin is doing via an unique method.
Revamp plugin main.go to illustrate the flag support, and add a getChain function to retrieve the chain data.
As a result, there's no longer the need for exporting newChainWithHomeFlags().
Update xgit.InitAndCommit() so it doesn't create a git repo if the path parameter is already inside a git repo.
This is useful for plugins when a repo is aimed to contain multiple plugins, we don't want the plugin scaffold command to create a subsidiary git repo inside the main git repo. This doesn't impact the scaffold chain command, unless it's invoked inside an existing git repo.
Change Execute* methods signature so they take a single ExecutedCommand or ExecutedHook argument.
This avoids mixing definition type Command and Hook and runtime type ExecutedCommand and ExecutedHook.
Improve error handling when the plugin returns an error.
test: replace custom plugin.Interface with a one generated by mockery.
test: update scaffolded plugin go.mod so it uses the cloned cli repo via a replace directive.
Before that change, the tests weren't using the current cli but the cli declared in the plugin go.mod.plush.

The field was passed to give access to the flags, but that wasb;t working because cobra.Command is not fully serializable (it has a lot of unexported fields).

tbruyelle · 2022-11-04T16:29:07Z

Converted to draft because I want to wait for #3038 to be merged, and then add flags support to hooks.

tbruyelle · 2022-11-09T13:19:08Z

@aljo242 it's ready now, sorry there's more changes than expected...

ignite/services/plugin/plugin.go

ignite/cmd/plugin.go

ignite/services/plugin/interface.go

ignite/pkg/cosmostxcollector/mocks/saver.go

joshLong145 · 2022-11-10T14:56:21Z

Observing that when a hook is invoked with ExecutedHook as the parameter the Command is properly hydrated but the Hook is always an empty struct. you can reproduce this with the example I have here: https://github.com/joshLong145/plugin-example. It's a strange bug as I would expect both to be empty definitions.

aljo242 · 2022-11-10T15:35:36Z

ignite/pkg/xgit/xgit.go

+// InitAndCommit creates a git repo in path if path isn't already inside a git
+// repository, then commits path content.
 func InitAndCommit(path string) error {


Do you think we can test this function?

Would just be tracked in a new issue. Not part of this scope.

sure it's possible, I've already created git repositories on the fly during the load plugin tests.

Can we create an issue for testing this in the future?

sure, done there #3125

tbruyelle · 2022-11-10T15:48:54Z

Observing that when a hook is invoked with ExecutedHook as the parameter the Command is properly hydrated but the Hook is always an empty struct. you can reproduce this with the example I have here: https://github.com/joshLong145/plugin-example. It's a strange bug as I would expect both to be empty definitions.

Indeed I can reproduce the problem, I didn't noticed it during my tests. Will try to find why, if you have any idea, tell me :)

ExecuteCommand cannot implement gob decoder/encoder and at the same time being embeded in ExecutedHook.

tbruyelle · 2022-11-10T16:29:33Z

@joshLong145 fixed 869bf71

The issue was because ExecutedCommand implements gob decoder/encoder and at the same time is embeded in ExecutedHook. In this particular situation it seems like gob takes the return of ExecuteCommand.GobDecode and put in ExecutedHook, without other kind of verification.

Thanks for spotting it!

joshLong145 · 2022-11-10T18:51:16Z

Observing that when a hook is invoked with ExecutedHook as the parameter the Command is properly hydrated but the Hook is always an empty struct. you can reproduce this with the example I have here: https://github.com/joshLong145/plugin-example. It's a strange bug as I would expect both to be empty definitions.

Indeed I can reproduce the problem, I didn't noticed it during my tests. Will try to find why, if you have any idea, tell me :)

trying to narrow it down myself. focusing on the creation of the struct during loadPluginHooks wondering if it has to do

@joshLong145 fixed 869bf71

The issue was because ExecutedCommand implements gob decoder/encoder and at the same time is embeded in ExecutedHook. In this particular situation it seems like gob takes the return of ExecuteCommand.GobDecode and put in ExecutedHook, without other kind of verification.

Thanks for spotting it!

that's interesting would not have thought it would work that way. I tested again it now is hydrating properly. 👍

* wip * wip: implements gob decoder/encoder * flags data are passing the network * hide flags field and expose Flags() func like cobra.Command * Remove cobra.Command field from plugin.Command The field was passed to give access to the flags, but that wasb;t working because cobra.Command is not fully serializable (it has a lot of unexported fields). * comments * add int64 flag type * test: add flags * test: plugin use current ignite/cli * chore: upgrade plugin CLI dep * fix: InitAndCommit doesn't create repo if already exists * chore: improve plugin default code * add CL entry * chore: update plugin CLI dep * fix after merge * dont remove band now * remove replace in plugin gomod and use latest version * fix: plugin.Hooks can return an error * update plugin dep version * add spinner during plugin scaffolding * fix: populate hook.with with plugin config * test: execute commands on linkPluginCmds * test: assert args in TestLinkPluginCmds * test: global args in TestLinkPluginHooks * refac(plugin): merge Commands and Hooks into Manifest Also stop using the same type for definition and execution. Now Manifest returns definition with type Command and Hook, while Execute* methods takes ExecutedCommand and ExecutedHook as parameters. * fix lint error * improve plugin error handling * feat(plugin): support more flag types * update plugin cli version and other fixes * test: try to fix * docs: comment * docs: more comments * docs: more comments * docs: comments * remove slack private lnk * remove private slack link * run make format * docs: add code comments * fix: ExecuteCommand embedding ExecuteCommand cannot implement gob decoder/encoder and at the same time being embeded in ExecutedHook. Co-authored-by: Alex Johnson <[email protected]>

tbruyelle added 12 commits November 4, 2022 14:34

wip

8250a82

wip: implements gob decoder/encoder

1a41498

flags data are passing the network

cc74492

hide flags field and expose Flags() func like cobra.Command

a5a6f76

Remove cobra.Command field from plugin.Command

67b2283

The field was passed to give access to the flags, but that wasb;t working because cobra.Command is not fully serializable (it has a lot of unexported fields).

comments

777c977

add int64 flag type

6e17c21

test: add flags

857f800

test: plugin use current ignite/cli

4cf7476

chore: upgrade plugin CLI dep

b63cb88

fix: InitAndCommit doesn't create repo if already exists

ef0bb8b

chore: improve plugin default code

3bdaa04

tbruyelle requested review from ilgooz, lumtis, jeronimoalbi, aljo242 and fadeev as code owners November 4, 2022 14:22

tbruyelle and others added 3 commits November 4, 2022 15:23

add CL entry

b772233

chore: update plugin CLI dep

701eb6a

Merge branch 'develop' into feat/plugin-cmd-flags

5cbb62b

aljo242 previously approved these changes Nov 4, 2022

View reviewed changes

joshLong145 mentioned this pull request Nov 4, 2022

feat: plugin system hooks updated #3038

Merged

Alex Johnson and others added 2 commits November 4, 2022 11:30

Merge branch 'develop' into feat/plugin-cmd-flags

dce5612

fix after merge

2e514d2

tbruyelle dismissed aljo242’s stale review via 2e514d2 November 4, 2022 15:47

aljo242 previously approved these changes Nov 4, 2022

View reviewed changes

dont remove band now

71a1417

tbruyelle dismissed aljo242’s stale review via 71a1417 November 4, 2022 16:09

tbruyelle marked this pull request as draft November 4, 2022 16:28

test: try to fix

c193512

aljo242 reviewed Nov 9, 2022

View reviewed changes

ignite/services/plugin/plugin.go Outdated Show resolved Hide resolved

tbruyelle added 3 commits November 9, 2022 15:23

docs: comment

d138126

docs: more comments

22bf4d0

docs: more comments

fb458c8

tbruyelle mentioned this pull request Nov 9, 2022

docs: Addition of plugin docs #3066

Merged

tbruyelle added 4 commits November 9, 2022 15:53

docs: comments

7345a0c

remove slack private lnk

0d1380c

Merge branch 'develop' into feat/plugin-cmd-flags

9d36ee7

remove private slack link

b2d4662

aljo242 reviewed Nov 9, 2022

View reviewed changes

ignite/cmd/plugin.go Show resolved Hide resolved

ignite/cmd/plugin.go Show resolved Hide resolved

ignite/services/plugin/interface.go Show resolved Hide resolved

ignite/pkg/cosmostxcollector/mocks/saver.go Outdated Show resolved Hide resolved

tbruyelle added 3 commits November 10, 2022 14:23

run make format

651e399

docs: add code comments

6bf925d

Merge branch 'develop' into feat/plugin-cmd-flags

f3ff026

aljo242 reviewed Nov 10, 2022

View reviewed changes

fix: ExecuteCommand embedding

869bf71

ExecuteCommand cannot implement gob decoder/encoder and at the same time being embeded in ExecutedHook.

Merge branch 'develop' into feat/plugin-cmd-flags

1dfc3de

Merge branch 'develop' into feat/plugin-cmd-flags

af6e9dd

aljo242 changed the base branch from develop to main November 15, 2022 13:52

Merge branch 'main' into feat/plugin-cmd-flags

50d1f34

aljo242 approved these changes Nov 15, 2022

View reviewed changes

Merge branch 'main' into feat/plugin-cmd-flags

e9b6346

aljo242 merged commit 184caaa into main Nov 15, 2022

aljo242 deleted the feat/plugin-cmd-flags branch November 15, 2022 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin): support command's flags #3060

feat(plugin): support command's flags #3060

tbruyelle commented Nov 4, 2022 •

edited

Loading

tbruyelle commented Nov 4, 2022

tbruyelle commented Nov 9, 2022

joshLong145 commented Nov 10, 2022

aljo242 Nov 10, 2022

tbruyelle Nov 10, 2022

aljo242 Nov 15, 2022

tbruyelle Nov 15, 2022

tbruyelle commented Nov 10, 2022

tbruyelle commented Nov 10, 2022 •

edited

Loading

joshLong145 commented Nov 10, 2022

feat(plugin): support command's flags #3060

feat(plugin): support command's flags #3060

Conversation

tbruyelle commented Nov 4, 2022 • edited Loading

tbruyelle commented Nov 4, 2022

tbruyelle commented Nov 9, 2022

joshLong145 commented Nov 10, 2022

aljo242 Nov 10, 2022

Choose a reason for hiding this comment

tbruyelle Nov 10, 2022

Choose a reason for hiding this comment

aljo242 Nov 15, 2022

Choose a reason for hiding this comment

tbruyelle Nov 15, 2022

Choose a reason for hiding this comment

tbruyelle commented Nov 10, 2022

tbruyelle commented Nov 10, 2022 • edited Loading

joshLong145 commented Nov 10, 2022

tbruyelle commented Nov 4, 2022 •

edited

Loading

tbruyelle commented Nov 10, 2022 •

edited

Loading